-
Notifications
You must be signed in to change notification settings - Fork 9
Revert s overflow handling #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert s overflow handling #13
Conversation
This reverts commit cbaa5f4.
/// Constructs two invalid signatures whose aggregate signature is valid | ||
#[test] | ||
fn test_aggregate_verify_strange() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the rest of the file ist rustfmt'd and this function does not look like it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed most of the stuff reported by rustfmt, but I don't agree with one suggestion in my code, and it also shows another suggestion in existing code. (Also happy to just accept all its suggestions if we want this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo the point of rustfmt is to avoid having to think about formatting at all. So I have a slight preference for just rustmt'ing everything. The other suggestion in existing code may just be a relic of forgetting to rustfmt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let me include this in the next PR, I have some local changes already on top of this PR.
Imo the point of rustfmt is to avoid having to think about formatting at all.
Yep, I think that's the advantage, and that's fine with me.
8e2c1cd
to
df69673
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK df69673
…ow handling df696736e26a8f6e03b9c04d1fde0ac7888337b1 halfagg: Note that invalid sigs can have valid aggsig and add test (Tim Ruffing) 64609172f401cf586f0f1335d1343119e0828d05 Revert "halfagg: fail inc_agg if input scalars overflow" (Tim Ruffing) Pull request description: ACKs for top commit: jonasnick: ACK df696736e26a8f6e03b9c04d1fde0ac7888337b1 Tree-SHA512: 84e70bf39533bd48f917f7e692f1422be54c86aae2d854cdd661e473d1a6a2f194e2b43779981c86196633985b98b061ce20c4cb30ba8280e3f3669c71638664
After some more consideration. I think the original behavior of ignoring overflows in input s values during aggregation was better. It simply removes an error path from the user. (For some background, BIP340 intentionally didn't distinguish between an invalid sig or an unparseable sig. This avoids having two distinct error paths where everyone just needs a single one.)
One could think that this change now could lead to cases where inputs sigs are invalid (due to overflow in s), but the resulting aggsig is valid. But first, such input sigs are computationally hard to create if the hash function is good, and second, we anyway can't hold up the property that invalid input sigs always produce an invalid aggsig (unless we verify inputs upfront), as we now note in the BIP.